Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces Box-based wrapper with a model-driven system: adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
44608ee to
ac99f10
Compare
745bf3d to
ed627c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/models/resource/test_resource.py (1)
143-156:test_overwritting_as_objectdoes not currently exercise an object-assignment path.The action in Line 155 still assigns a dict, so this test overlaps heavily with
test_overwritting. Consider renaming or changing the assignment to an object instance to match intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/resource/test_resource.py` around lines 143 - 156, The test test_overwritting_as_object is claiming to test object-assignment but still assigns a dict to agreement.parameters.ordering[1]; change the assignment to instantiate and assign an actual parameter-item object instead of a dict (e.g., create an instance of the ordering item class used by AgreementDummy or the model class that represents ordering entries) so the code path for object replacement is exercised; update the assignment target agreement.parameters.ordering[1] to use that instantiated object and adjust any assertions if needed to compare object attributes rather than raw dicts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/models/model.py`:
- Around line 177-183: The Model.__init__ currently mutates the caller's
resource_data dict and allows payload keys like "meta" or "resource_data" to
collide with constructor params; to fix, make a new dict copy for merging (e.g.,
data = dict(resource_data) if resource_data else {}) so the caller's object is
not modified, then check kwargs for reserved names ("meta", "resource_data") and
either remove them or raise a clear TypeError to prevent silent argument binding
collisions, finally update the new data dict with kwargs and call
super().__init__(**data); reference the __init__ method, the "meta" and
"resource_data" keys, and the Model constructor when applying this change.
---
Nitpick comments:
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-156: The test test_overwritting_as_object is claiming to test
object-assignment but still assigns a dict to agreement.parameters.ordering[1];
change the assignment to instantiate and assign an actual parameter-item object
instead of a dict (e.g., create an instance of the ordering item class used by
AgreementDummy or the model class that represents ordering entries) so the code
path for object replacement is exercised; update the assignment target
agreement.parameters.ordering[1] to use that instantiated object and adjust any
assertions if needed to compare object attributes rather than raw dicts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1c512842-de4b-4639-b374-fb275dede14a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
ed627c4 to
bdb6d5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mpt_api_client/models/model.py (1)
177-183:⚠️ Potential issue | 🟠 Major
Model.new/Model.__init__still allow key collisions and caller-data mutation.This is still unresolved: payload splatting in
newcan collide with reserved constructor params (meta,resource_data), and__init__can mutate caller-ownedresource_datawhen kwargs are merged.💡 Proposed fix
class Model(BaseModel): @@ def __init__( self, resource_data: ResourceData | None = None, meta: Meta | None = None, **kwargs: Any ) -> None: object.__setattr__(self, "meta", meta) - data = resource_data or {} + data = dict(resource_data or {}) data.update(kwargs) super().__init__(**data) @@ def new(cls, resource_data: ResourceData | None = None, meta: Meta | None = None) -> Self: """Creates a new resource from ResourceData and Meta.""" - return cls(meta=meta, **(resource_data or {})) + return cls(resource_data=resource_data, meta=meta)As per coding guidelines, “For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved.”
Also applies to: 191-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 177 - 183, Model.new and Model.__init__ currently allow payload splatting to collide with reserved params (meta, resource_data) and mutate caller-owned resource_data when merging kwargs; fix by defensively copying and validating inputs: in Model.__init__ (and in Model.new) make a shallow copy of the incoming resource_data (use a new dict) before merging, explicitly disallow or raise if kwargs contain reserved keys ("meta" or "resource_data") to prevent collisions, and perform the merge into the new copy rather than mutating the caller's dict; update Model.new to filter/validate its payload before forwarding to Model.__init__ (or call __init__ with explicit args) so reserved params cannot be overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/models/model.py`:
- Around line 140-164: The _process_value method in BaseModel is too complex;
extract the dict and list branches into small helpers: implement a
_process_dict_value(self, value: dict, target_class: Any) -> Any that contains
the dict-to-BaseModel logic (use target_class if issubclass(BaseModel) else
BaseModel), a _resolve_list_model_class(self, target_class: Any) ->
Type[BaseModel] that encapsulates get_origin/get_args logic to pick the element
model, and a _process_list_value(self, value: Iterable, target_class: Any) ->
ModelList that calls _resolve_list_model_class and returns ModelList(value,
model_class=...), then replace the dict/list blocks in _process_value with calls
to these helpers so functionality (including checks for BaseModel, ModelList,
UserList) remains identical but cognitive complexity is reduced.
---
Duplicate comments:
In `@mpt_api_client/models/model.py`:
- Around line 177-183: Model.new and Model.__init__ currently allow payload
splatting to collide with reserved params (meta, resource_data) and mutate
caller-owned resource_data when merging kwargs; fix by defensively copying and
validating inputs: in Model.__init__ (and in Model.new) make a shallow copy of
the incoming resource_data (use a new dict) before merging, explicitly disallow
or raise if kwargs contain reserved keys ("meta" or "resource_data") to prevent
collisions, and perform the merge into the new copy rather than mutating the
caller's dict; update Model.new to filter/validate its payload before forwarding
to Model.__init__ (or call __init__ with explicit args) so reserved params
cannot be overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9a6ca653-f8b8-44db-997d-e8434c3c2d81
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mpt_api_client/models/model.py (1)
140-164:⚠️ Potential issue | 🟠 Major
_process_valuestill exceeds the cognitive complexity gate.Line 140 keeps the same high-branching structure; Sonar still reports this as above threshold. Please split dict/list/type-resolution into helpers and keep
_process_valueas a dispatcher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 140 - 164, Refactor _process_value by moving the dict and list handling into small helpers: create _process_dict_value(value, target_class) to encapsulate the dict->BaseModel logic (including the issubclass check and returning target_class(**value) or BaseModel(**value), and create _resolve_list_model_class(target_class) to return the element model_class (default BaseModel) by inspecting get_origin/get_args, and _process_list_value(value, target_class) to call that resolver and return ModelList(value, model_class=model_class); then make _process_value simply dispatch to these helpers (dict -> _process_dict_value, list/UserList -> _process_list_value, else return value) so branching is isolated into the new functions while preserving existing behavior and using the same symbols: _process_value, _process_dict_value, _process_list_value, _resolve_list_model_class, BaseModel, ModelList.
🧹 Nitpick comments (1)
tests/unit/models/resource/test_resource.py (1)
143-160:test_overwritting_as_objectdoes not currently test object-style mutation.Line 155 still performs dict assignment (same path as the prior test), so the object-mutation path remains untested. Consider mutating
agreement.parameters.ordering[1].value(or similar) and renaming the typo’doverwrittingtests.💡 Suggested adjustment
-def test_overwritting_as_object(): +def test_overwriting_as_object(): @@ - agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act + agreement.parameters.ordering[1].value = "Evergreen Terrace" # act + agreement_data["parameters"]["ordering"][1]["value"] = "Evergreen Terrace" @@ - assert agreement.parameters.ordering[1].external_id == "address" + assert agreement.parameters.ordering[1].external_id == "address" + assert agreement.parameters.ordering[1].value == "Evergreen Terrace"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/resource/test_resource.py` around lines 143 - 160, Rename the test and change the mutation to exercise the object-style path: in test_overwritting_as_object (or better test_overwriting_as_object) instead of assigning a dict to agreement.parameters.ordering[1], mutate the attribute on the item object (e.g., agreement.parameters.ordering[1].value = "Springfield" or change .external_id/.value as appropriate) so you exercise the object-mutation code path on AgreementDummy and its nested list; keep assertions for agreement.id and the ordering items' attributes (use .external_id/.value accessors) and ensure agreement.to_dict() still equals the original agreement_data after the object mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/models/model.py`:
- Around line 14-24: The acronym regex _SNAKE_CASE_ACRONYM should not use an
atomic group because it blocks backtracking and breaks conversions like
"IPAddress" and "HTTPServerError"; replace its definition with a plain capturing
pattern such as re.compile(r"([A-Z]+)([A-Z][a-z0-9])") so to_snake_case's
subsequent substitution _SNAKE_CASE_ACRONYM.sub(r"\1_\2", snake) correctly
inserts underscores between acronym runs and the following capitalized word (no
other changes to to_snake_case or the substitution logic required).
---
Duplicate comments:
In `@mpt_api_client/models/model.py`:
- Around line 140-164: Refactor _process_value by moving the dict and list
handling into small helpers: create _process_dict_value(value, target_class) to
encapsulate the dict->BaseModel logic (including the issubclass check and
returning target_class(**value) or BaseModel(**value), and create
_resolve_list_model_class(target_class) to return the element model_class
(default BaseModel) by inspecting get_origin/get_args, and
_process_list_value(value, target_class) to call that resolver and return
ModelList(value, model_class=model_class); then make _process_value simply
dispatch to these helpers (dict -> _process_dict_value, list/UserList ->
_process_list_value, else return value) so branching is isolated into the new
functions while preserving existing behavior and using the same symbols:
_process_value, _process_dict_value, _process_list_value,
_resolve_list_model_class, BaseModel, ModelList.
---
Nitpick comments:
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-160: Rename the test and change the mutation to exercise the
object-style path: in test_overwritting_as_object (or better
test_overwriting_as_object) instead of assigning a dict to
agreement.parameters.ordering[1], mutate the attribute on the item object (e.g.,
agreement.parameters.ordering[1].value = "Springfield" or change
.external_id/.value as appropriate) so you exercise the object-mutation code
path on AgreementDummy and its nested list; keep assertions for agreement.id and
the ordering items' attributes (use .external_id/.value accessors) and ensure
agreement.to_dict() still equals the original agreement_data after the object
mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1a05f83d-5ca8-44af-a145-16e814bd0efe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
bdb6d5e to
841cf34
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpt_api_client/models/model.py (1)
205-207:⚠️ Potential issue | 🟠 MajorGuard
_data_keyextraction before calling.get.Line 206 assumes a dict when
_data_keyis set. If the payload is non-dict, this raisesAttributeErrorinstead of the intendedTypeError.Proposed fix
if isinstance(response_data, dict): response_data.pop("$meta", None) if cls._data_key: - response_data = response_data.get(cls._data_key) + if not isinstance(response_data, dict): + raise TypeError("Response data must be a dict.") + response_data = response_data.get(cls._data_key) if not isinstance(response_data, dict): raise TypeError("Response data must be a dict.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 205 - 207, The code calls response_data.get(cls._data_key) when cls._data_key is set, which will raise AttributeError if response_data is not a dict; update the block in the class method that handles response_data so you first check isinstance(response_data, dict) before attempting response_data.get(...), and if it's not a dict raise the intended TypeError (or handle None appropriately) with a clear message; change the logic around the symbols cls._data_key and response_data to perform the type guard prior to extraction and preserve existing behavior when the key is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/models/model.py`:
- Line 5: The import of typing.override in mpt_api_client/models/model.py will
fail on Python 3.12; replace it with a conditional/fallback import so older
Pythons use typing_extensions.override (or import override from typing when
available) — update the import line that currently references "override"
alongside "Any, ClassVar, Self, get_args, get_origin" to try importing override
from typing_extensions first (or fall back to typing for 3.13+), and ensure
packaging includes typing-extensions if you choose the stable dependency route.
- Around line 69-70: The BaseModel class is annotated with `@dataclass` which
auto-generates __eq__ for zero fields causing all BaseModel instances to compare
equal; update the class declaration to disable dataclass equality (e.g., change
`@dataclass` to `@dataclass`(eq=False)) or remove the `@dataclass` decorator entirely,
ensuring existing dynamic attribute handling via object.__setattr__ remains
unchanged; adjust any imports/usages if you remove the decorator and run tests
focusing on BaseModel, _process_value, and places that instantiate BaseModel to
confirm equality semantics are fixed.
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-160: The test test_overwritting_as_object is supposed to
exercise replacing an ordering element with an object but currently assigns a
dict (duplicating test_overwritting); change the assignment to instantiate the
same element class as the existing item instead of a plain dict. For example,
replace the line agreement.parameters.ordering[1] = {"externalId": "address",
"value": "Springfield"} with something that constructs the same type as
agreement.parameters.ordering[1] (e.g. agreement.parameters.ordering[1] =
type(agreement.parameters.ordering[1])("address", "Springfield") or use the
appropriate constructor/class for parameter items) so the test uses object
reassignment while keeping the rest of assertions and AgreementDummy usage
intact.
---
Outside diff comments:
In `@mpt_api_client/models/model.py`:
- Around line 205-207: The code calls response_data.get(cls._data_key) when
cls._data_key is set, which will raise AttributeError if response_data is not a
dict; update the block in the class method that handles response_data so you
first check isinstance(response_data, dict) before attempting
response_data.get(...), and if it's not a dict raise the intended TypeError (or
handle None appropriately) with a clear message; change the logic around the
symbols cls._data_key and response_data to perform the type guard prior to
extraction and preserve existing behavior when the key is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 95a8615a-c598-4098-81be-bfb97fb07713
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
| def test_overwritting_as_object(): | ||
| agreement_data = { | ||
| "id": "AGR-123", | ||
| "parameters": { | ||
| "ordering": [ | ||
| {"externalId": "contact", "value": "Hommer Simpson"}, | ||
| {"externalId": "address", "value": "Springfield"}, | ||
| ] | ||
| }, | ||
| } | ||
| agreement = AgreementDummy(agreement_data) | ||
|
|
||
| agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act | ||
|
|
||
| assert agreement.id == "AGR-123" | ||
| assert agreement.parameters.ordering[0].external_id == "contact" | ||
| assert agreement.parameters.ordering[1].external_id == "address" | ||
| assert agreement.to_dict() == agreement_data |
There was a problem hiding this comment.
test_overwritting_as_object currently duplicates dict-path coverage.
This test name suggests object reassignment, but Line 155 still assigns a dict, so it overlaps with test_overwritting and misses the intended branch.
Proposed test adjustment
def test_overwritting_as_object():
@@
- agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act
+ replacement = agreement.parameters.ordering[1]
+ agreement.parameters.ordering[1] = replacement # act: object-path reassignment
@@
assert agreement.parameters.ordering[1].external_id == "address"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/models/resource/test_resource.py` around lines 143 - 160, The test
test_overwritting_as_object is supposed to exercise replacing an ordering
element with an object but currently assigns a dict (duplicating
test_overwritting); change the assignment to instantiate the same element class
as the existing item instead of a plain dict. For example, replace the line
agreement.parameters.ordering[1] = {"externalId": "address", "value":
"Springfield"} with something that constructs the same type as
agreement.parameters.ordering[1] (e.g. agreement.parameters.ordering[1] =
type(agreement.parameters.ordering[1])("address", "Springfield") or use the
appropriate constructor/class for parameter items) so the test uses object
reassignment while keeping the rest of assertions and AgreementDummy usage
intact.
841cf34 to
c53c5b4
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/unit/models/resource/test_resource.py (1)
143-160:⚠️ Potential issue | 🟡 Minor
test_overwritting_as_objectstill does not test object reassignment.Line 155 assigns a dict again, so this test duplicates
test_overwrittinginstead of exercising the object-path mutation it is named for.💡 Proposed fix
- agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act + replacement = agreement.parameters.ordering[1] + agreement.parameters.ordering[1] = replacement # actAs per coding guidelines:
For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/resource/test_resource.py` around lines 143 - 160, The test test_overwritting_as_object currently reassigns agreement.parameters.ordering[1] to a dict (duplicating test_overwritting); update it to reassign with an actual object instance that mimics the resource wrapper (e.g., construct the same type used for items in ordering or a simple object with attributes external_id and value) so the object-path mutation is exercised; keep assertions (agreement.id, agreement.parameters.ordering[0].external_id, agreement.parameters.ordering[1].external_id and agreement.to_dict()) the same to verify the object assignment round-trips, and use AgreementDummy / agreement.parameters.ordering to locate the change.mpt_api_client/models/model.py (1)
69-70:⚠️ Potential issue | 🟠 MajorDisable dataclass-generated equality on
BaseModel.At Line 69,
@dataclassauto-generates__eq__; with dynamic attributes and no dataclass fields, equality can become incorrect for model instances. Use@dataclass(eq=False)(or remove@dataclass) to avoid false-equal comparisons.💡 Proposed fix
-@dataclass +@dataclass(eq=False) class BaseModel:#!/bin/bash set -euo pipefail # Verify current declaration in repository rg -n '^\s*@dataclass|^\s*class\s+BaseModel\b' mpt_api_client/models/model.py -C1 # Verify Python dataclass equality behavior (standalone, no repo code execution) python - <<'PY' from dataclasses import dataclass `@dataclass` class Base: pass class Child(Base): def __init__(self, **kwargs): self.__dict__.update(kwargs) print("Child(a=1) == Child(b=2):", Child(a=1) == Child(b=2)) PYAs per coding guidelines:
For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 69 - 70, The BaseModel is decorated with `@dataclass` which generates an automatic __eq__ that can produce incorrect comparisons for instances with dynamic attributes; change the decorator to `@dataclass`(eq=False) (or remove the dataclass decorator) on the BaseModel class declaration so dataclass-generated equality is disabled for BaseModel (locate the class by the symbol BaseModel and the decorator `@dataclass` and update it to `@dataclass`(eq=False) to prevent false-equal comparisons).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mpt_api_client/models/model.py`:
- Around line 69-70: The BaseModel is decorated with `@dataclass` which generates
an automatic __eq__ that can produce incorrect comparisons for instances with
dynamic attributes; change the decorator to `@dataclass`(eq=False) (or remove the
dataclass decorator) on the BaseModel class declaration so dataclass-generated
equality is disabled for BaseModel (locate the class by the symbol BaseModel and
the decorator `@dataclass` and update it to `@dataclass`(eq=False) to prevent
false-equal comparisons).
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-160: The test test_overwritting_as_object currently reassigns
agreement.parameters.ordering[1] to a dict (duplicating test_overwritting);
update it to reassign with an actual object instance that mimics the resource
wrapper (e.g., construct the same type used for items in ordering or a simple
object with attributes external_id and value) so the object-path mutation is
exercised; keep assertions (agreement.id,
agreement.parameters.ordering[0].external_id,
agreement.parameters.ordering[1].external_id and agreement.to_dict()) the same
to verify the object assignment round-trips, and use AgreementDummy /
agreement.parameters.ordering to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0275b50b-591e-4b6b-a96c-a70e300c0b0e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
0eeb456 to
cc78f0b
Compare
|
Replaced box for dataclasses
cc78f0b to
7ba3568
Compare



Closes MPT-18701